Skip to content

Make every command behave the same from every invocation source#612

Merged
Juliusolsson05 merged 7 commits into
mainfrom
feat/command-invocation-parity
Jul 27, 2026
Merged

Make every command behave the same from every invocation source#612
Juliusolsson05 merged 7 commits into
mainfrom
feat/command-invocation-parity

Conversation

@Juliusolsson05

Copy link
Copy Markdown
Owner

Started from a user report that ⌥P (Prompt Template…) does nothing. Six classification agents then swept all 99 catalog commands and the surface/store wiring. ⌥P turned out to be one of nine.

The rule this establishes

Invoking a command that shows a surface makes it visible; invoking it again dismisses it — from every invocation source, not just from inside the palette.

Three independent layers broke that, and no single-layer fix is sufficient: a toggle written in a command body is unreachable if the router refuses the second keypress, and a router fix is pointless if the command can't ask whether it's already open.

What was broken

Nine commands were silent no-ops from chord and native menu. Two shipped default chords, so they were dead from the keyboard in the released app: resume-session (⌘⇧R) and prompt-template (⌥P). A chord doesn't open the palette — it mounts it invisibly, runs the command, and unmounts in the same commit, so ui.enterXMode() wrote React state to a component about to be destroyed. A second mechanism would have killed it anyway: the mount reset effect is passive, so it runs after the layout effect that dispatched and queues 'commands' last.

keepPaletteOpen: true never helped on that path. It's read only by the palette's own execute path — nine commands have been declaring a flag that does nothing outside the palette.

⌘⇧R is a regression from #608, which moved the chord off a hard-coded router branch onto the command.

Ten surfaces were open-only. The store knew whether they were open and every close* action existed; neither was reachable from a command. Only closePalette was on the bridge.

Dialog-based surfaces couldn't be dismissed by their own chord at all. useKeybinds bails out of the whole handler while any Radix dialog is mounted. That gate is load-bearing — without it ⌘W closes a pane behind a confirmation dialog — but it's why ⌥R Reader round-trips (renders inline) and ⌘⇧U Usage doesn't (dialog). The difference was never in the commands.

The editor guards were transposed. save-editor-file carried globalEditorOpen || focusedCwd plus a comment describing ⌘⌥E fullscreen; toggle-editor-fullscreen carried when: globalEditorOpen, which runs during admission and made its own open-into-fullscreen branch unreachable. Each broke in its own direction: ⌘⌥E with the editor closed was a silent no-op, and File → Save with the editor closed was admitted and did nothing.

What changed

Phase Commit
5 1319dbba Un-transpose the editor guards, scope buried admission to the active tab, idempotent openGlobalEditor, drop 8 dead ui members
1 34c600c2 Palette sub-mode → store; fixes all nine no-ops
2 f342a6fd Narrow exemption letting a chord dismiss the surface its own command owns
3+4 8ac304b3 Ten surface flags + close actions on the bridge; toggle conversions with getState

Decisions worth reviewing

Store-backed mode over a smaller patch. The three-edit alternative touches fewer files and has three parts that must all be present, with a correctness argument resting on React effect ordering. The store version has no ordering argument at all.

A table, not a derived predicate, for surface ownership. Asking the command (getState(ctx)?.value === 'on') couldn't drift — and needs a CommandContext, which assembles ~76 workspace actions. Deferring that is why the router forwards an id instead of dispatching inline (#494). Drift is bounded by keyof UiShellState instead.

Deliberately not toggled, pinned by test: per-target pickers (a second press should re-target, not close), save-debug-logs / attach-recording-note (each writes something on the way in), open-settings (destination, not peek), the palette itself (double-tap flicker).

Verification

  • tsc -b clean after rm -rf .tsc-out
  • check:keybindings OK — 39 binding sets, 13 reserved, 5 approved overlaps
  • 246/246 test files
  • Palette-mode tests sabotage-checked: removing the commandPaletteOpen coupling fails two cases
  • The surface-ownership test failed on first run and caught a real gap (usage.open toggled with no badge)

Known remaining, documented in the plan

RemotePanel still renders a Dialog while registered as a side panel — now cosmetic rather than functional. Escape still doesn't dismiss Settings (dead code behind the ownership gate; separate concern since Settings is excluded from toggling by design).

🤖 Generated with Claude Code

Juliusolsson05 and others added 7 commits July 26, 2026 23:22
Six classification agents swept all 99 catalog commands and the surface
wiring after a user report that Alt+P does nothing.

The rule this establishes: invoking a command that shows a surface makes it
visible; invoking it again dismisses it -- from EVERY invocation source, not
just from inside the palette.

Three independent layers break that today, and no single-layer fix is
sufficient. A toggle written in a command body is unreachable if the router
refuses the second keypress; a router fix is pointless if the command has no
way to ask whether it is already open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, drop dead bridge members

Seven discrete defects the classification sweep turned up. Independent of
the three activation layers the plan describes; landing first because one
of them is a regression.

THE EDITOR GUARDS WERE SWAPPED. `save-editor-file` carried
`globalEditorOpen || focusedCwd` plus a comment describing Cmd+Alt+E
fullscreen, and `toggle-editor-fullscreen` carried `when: globalEditorOpen`.
Each broke in its own direction:

  - Cmd+Alt+E with the editor CLOSED was a silent no-op. `when` runs during
    admission, before `run`, so the branch that opens the editor straight
    into fullscreen was unreachable from all four invocation sources. That
    branch is the intended behavior -- the migrated comment says so
    outright: "the chord has always meant 'give me a big editor' as ONE
    gesture... or routing would silently drop half the feature." Routing
    did. This is a regression from the governance PR, which moved the chord
    off a hard-coded keybind branch onto the command.
  - File -> Save with the editor closed was ADMITTED and did nothing.
    `requestSaveActiveEditorFile` dispatches a window event whose only
    listener is EditorWorkbench, which does not mount until the editor is
    open.

Each guard now sits on the command it describes, with the comment.

OPEN-IF-CLOSED IS NOW IDEMPOTENT. `ui.toggleGlobalEditor` was the only
editor primitive on the command bridge, so four call sites wanting "make
sure the editor is up" each wrote read-then-toggle against `flags
.globalEditorOpen` -- a value snapshotted when the CommandContext memo was
built. A stale `true` CLOSES the editor the user just asked to open.
`openGlobalEditor`/`closeGlobalEditor` already existed on the store; they
are now on the bridge, and all four sites use them.

BURIED ADMISSION AGREES WITH THE LIST. `revive-pane` and `kill-buried-pane`
checked `state.buried.length > 0` across the whole workspace while the
picker filters by `sourceTabId`, so both could be admitted from a tab with
nothing buried and land on an empty list.

DEAD BRIDGE MEMBERS REMOVED. Eight members of `CommandContext.ui` had zero
command call sites: openResumePicker, openAgentStatusPanel,
closeAgentStatusPanel, toggleStatusMode, toggleWorktreeBadges,
toggleUsageHeader, cycleUsageHeaderLevel, setDangerousAgentsEnabled. Their
store subscriptions, object-literal entries and memo deps went with them --
removing the type alone would have left exactly the mechanism-with-no-
consumer shape this whole sweep exists to find. Plus a dead
`toggleCommandPalette` subscription in App.tsx and three unused
commandState imports.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nine commands did nothing when invoked by a keybinding or the native menu.
Two of them ship default chords, so they were dead from the keyboard in the
shipped app: Resume Session (Cmd+Shift+R) and Prompt Template (Alt+P). The
user reported Alt+P; the sweep found the other eight.

The family is every command whose `run` calls `ui.enterXMode()` — the two
buried commands, the three prompt-template ones, the three AI-workspace
ones, and Resume Session.

WHY THEY FAILED. A chord does not open the palette. It sets
`pendingCommandInvocation`, which mounts the palette host INVISIBLY
(`visible={false}`), runs the command, and unmounts it in the same commit.
`ui.enterXMode` was a `useState` setter on that host, so the mode was
written to a component that was already invisible and about to be
destroyed.

A second mechanism would have killed it even if the host had survived: the
mount-time reset effect calls `setMode('commands')`, and passive effects
run AFTER layout effects in the same commit, so 'commands' was queued last
and won. StrictMode double-invokes it for good measure.

`keepPaletteOpen: true` never helped and was never going to. It is read
only by the palette's OWN executeCommand; the chord path goes through
`dispatchCommand`, which has no such branch. Nine commands have been
declaring a flag that does nothing outside the palette.

THE FIX. `paletteMode` becomes uiShell state, and `setPaletteMode` also
sets `commandPaletteOpen: true` — the open is the correctness fix, not a
convenience, because a sub-mode is only meaningful rendered and every
caller wanted the palette up. Coupling them in the action means a future
mode-entry point cannot forget it. Closing resets the mode, so reopening
never resumes an abandoned sub-flow.

Store state has no component lifecycle, so neither mechanism can reach it.
That is the whole argument -- there is no effect-ordering claim to get
wrong. The tempting three-edit alternative (open the palette in `run`,
extend the excluded-id set, skip the reset when an invocation was pending)
was rejected for the opposite reason: omitting any one part leaves the bug,
and its correctness rests entirely on React's effect ordering.

Two consequences worth noting. The mount reset now skips entirely when the
mount was caused by a pending invocation, because that command may have
seeded state synchronously and the passive reset would wipe it. And the
close-after-run rule now asks the LIVE flag — "did this command turn the
palette on?" — instead of consulting a hardcoded id list that held only
`open-command-palette` and could never have covered these nine.

Tests drive the store rather than the component, deliberately: the bug was
not a rendering bug, it was state having a lifecycle it should not have had.
Verified they fail when `setPaletteMode` stops opening the palette.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`useKeybinds` bails out of the entire key handler while any Radix dialog is
mounted. That gate is correct and load-bearing -- without it, Cmd+W closes a
pane behind an open confirmation dialog -- but it also means a surface built
as a dialog makes ITS OWN chord unreachable while it is up.

That is the whole explanation for why some chords round-trip and some do
not, and none of it was in the commands:

  Alt+R Reader, Alt+S Spotlight   round-trip   render inline, no owner marker
  Agent Status                    round-trips  plain <aside>
  Cmd+Shift+U Usage               dead         Radix dialog
  Remote Panel                    dead         Radix dialog, though its store
                                               action is already a correct toggle

Without this commit, every toggle written in a command body would be
unreachable by keyboard: the second press never reaches the binding router,
so `run` is never called.

THE EXEMPTION IS ONE CASE. A chord crosses the gate only when it maps to
the command that owns the surface currently holding the interaction.
Pressing Cmd+Shift+U while Usage is up is unambiguously "dismiss Usage" --
it cannot be a stray shortcut leaking into a modal, because the modal is the
thing that command controls. Context is restricted to 'global': a surface
owns the screen, so a grid- or dispatch-scoped binding has no business
firing underneath it.

WHY A TABLE rather than asking the command via `getState(ctx)?.value ===
'on'`, which would be derived and could not drift: evaluating `getState`
needs a CommandContext, and building one assembles ~76 workspace actions.
Deferring exactly that cost is why the router forwards an id through
`requestCommandInvocation` instead of dispatching inline (#494). Paying it
on every keystroke that lands while a dialog is open would be a real
regression traded for a keyboard nicety. Drift is bounded by types instead:
the value is `keyof UiShellState`, and the commands read the same flag
through `CommandContext.flags`.

The table deliberately excludes per-target pickers (a second press should
RE-TARGET, not close), the two commands that write something on the way in
(Save Debug Logs, Attach Recording Note), the Settings page (a destination,
not a peek), and the palette itself. Those exclusions are pinned by a test,
so dropping one later is a decision rather than an accident.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ss it

Ten surfaces were open-only: invoking the command again did nothing,
because the command had no way to ask whether it was already open. The
store knew, and every `close*` action already existed -- neither was
reachable from a command. Only `closePalette` was on the bridge.

Usage, Keyboard Shortcuts, Agent Activity, Close Old Agents, Switch Agents,
Prompt Search, Remote Panel, Reorder Tabs, Pin Agents and the New Tab path
picker now round-trip, each following the `tiled-tabs` pattern that was
already in the tree: read the flag, branch, call the one-directional action.
These surfaces have no toggle action of their own -- open and close are
separate store actions -- so the command is where the decision has to live.

Every flag is THREE edits: the `flags` type, the context object literal,
and the memo dependency array. Missing the third is a silent staleness bug
-- the flag is read once and never updates -- so all three are done for all
ten, and the count is visible in the diff.

WHAT IS DELIBERATELY NOT HERE. The nine per-target surfaces store a
`SessionId | null`, not a boolean, and collapsing that would lose the case
that matters: pressing the chord while View Prompts is open for a DIFFERENT
session must re-target, not close. Save Debug Logs and Attach Recording Note
each write something on the way in, so a second press legitimately writes a
second one. Settings is a destination, not a peek. The palette itself would
flicker on a double-tap. Those exclusions are pinned by a test rather than
left implicit.

`toggle-remote-panel` needed only the badge: its store action was already a
correct toggle, but `remotePanelOpen` was the one panel flag missing from
`flags`, making it the sole panel command in the app with no Open/Closed
state while eleven siblings had one.

The new test asserts every command in the surface-ownership table has a
`getState`. It failed on first run and caught a real gap -- `usage.open`
toggled but reported nothing -- which is the point: the table lets a chord
cross the interaction gate, and the command's own state and run are what
dismiss once it does. A command listed there without a badge is one whose
second press reaches a `run` that may still only open, which is precisely
the reported bug wearing a different hat.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two orchestrated reviewers read the branch independently. Codex: 2 high, 0
critical. Claude: 10 findings, none breaking types or tests. All valid, all
fixed.

THE RULE WAS DELIVERED FOR HALF THE BUG REPORT. Phase 1 turned Alt+P and
Cmd+Shift+R from *nothing* into *opens*; the second press still did nothing,
because the palette is itself a dialog and no palette-mode command was in the
ownership table -- so Phase 2, which exists precisely to make a dialog-backed
surface dismissable by its own chord, skipped the two chords the whole change
started from. Alt+P's second press was worse than inert: Alt chords over a text
target are not preventDefault-ed, so macOS composed a pi into the palette query.

The nine now compare `flags.paletteMode` and dismiss. They live in a SECOND
table, and that split was found by a failing test rather than by design: merging
both kinds of ownership into one map type-checked, and implied Resume Session
should carry an Open/Closed badge. It should not -- "the palette is open" is not
a property of Resume Session. These commands do not own a surface with its own
flag; they own a MODE of one shared surface.

PHASE 2 ONLY EXEMPTED KEYBINDINGS. The native-menu handler had its own copy of
the ownership bail, so File -> New Tab twice left the picker up. "Every
invocation source" was false for the menu, in a PR whose title is that phrase.

THE TABLE'S TYPE DID NOT DO WHAT ITS COMMENT CLAIMED. `keyof UiShellState`
accepts any of ~40 keys including the nine `SessionId | null` ones, where
`=== true` is permanently false -- silent, and exactly the failure the module's
own test says it guards. Narrowed to boolean-valued keys; sabotage-verified that
a nullable flag now fails to compile.

THE MOUNT-RESET EFFECT WAS UNREACHABLE. `mountedForPendingCommand` is never
false: `openCommandPalette`'s only caller is a command, and commands run from
inside an already-mounted host. Its state resets were redundant on a fresh
mount, but its focus call was not -- focus now keys on `visible`.

new-tab REMOVED from the toggle set: `pathPickerOpen` means "the shared path
modal is open", not "the new-tab picker is open", which is the same
boolean-loses-information problem the session-scoped surfaces were kept out for.
Live consequences settled it -- Cmd+T dismissed the picker mid-submit during a
slow spawn, and a create action had started rendering an Open/Closed badge.

Plus: `toggleCommandPalette` deleted (Phase 5 removed its only reader and Phase 1
then gave it new behavior -- mechanism-with-no-consumer, inside the sweep for
that shape); `openResumePicker`'s removal had left a destructure and a memo dep
behind; the live-flag close rule holds only for a synchronous open and now says
so; one dep array was wrong and misindented, in the commit about dep arrays.

New test: nothing proved a toggle's `run` closed the RIGHT surface. It drives
every listed command with its own flag true and asserts exactly one dismissal
and no open. It caught a real distinction on first run -- Remote Panel
legitimately uses a `toggle*` rather than a `close*`.

tsc -b --force clean, check:keybindings OK (39 sets), 246/246 test files.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Juliusolsson05
Juliusolsson05 merged commit 232c86a into main Jul 27, 2026
1 check passed
@Juliusolsson05
Juliusolsson05 deleted the feat/command-invocation-parity branch July 27, 2026 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant